Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compatibility with Sulu 2.5 - Symfony 6 #162

Merged
merged 8 commits into from
Apr 25, 2024

Conversation

dev-newvisibility
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? yes
Related issues/PRs #159
License MIT

What's in this PR?

Compatibility with Sulu 2.5 - Symfony 6

BC Breaks/Deprecations

Removed compatibility with Swiftmailer (replaced with Symfony/Mailer)
Removed compatibility with Symfony 4.4

@Jupi007
Copy link
Contributor

Jupi007 commented Feb 25, 2023

Hi,
I'm testing this bundle with Sulu2.5 / Sf6.2 through you fork.

The only issue I can report for now is a problem when uploading an avatar.
It throw an exception in the SaveMediaTrait:

Symfony\Component\ErrorHandler\Error\UndefinedMethodError:
Attempted to call an undefined method named "get" of class "Sulu\Bundle\CommunityBundle\Controller\ProfileController".
Did you mean to call e.g. "getSubscribedServices", "getSubscribedServicesOfSaveMediaTrait" or "getUser"?

  at vendor/sulu/community-bundle/Controller/SaveMediaTrait.php:112
  at Sulu\Bundle\CommunityBundle\Controller\ProfileController->getMediaManager()
     (vendor/sulu/community-bundle/Controller/SaveMediaTrait.php:87)
  at Sulu\Bundle\CommunityBundle\Controller\ProfileController->saveMedia()
     (vendor/sulu/community-bundle/Controller/SaveMediaTrait.php:78)
  at Sulu\Bundle\CommunityBundle\Controller\ProfileController->saveAvatar()
     (vendor/sulu/community-bundle/Controller/SaveMediaTrait.php:28)
  at Sulu\Bundle\CommunityBundle\Controller\ProfileController->saveMediaFields()
     (vendor/sulu/community-bundle/Controller/ProfileController.php:59)
  at Sulu\Bundle\CommunityBundle\Controller\ProfileController->indexAction()
     (vendor/symfony/http-kernel/HttpKernel.php:163)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw()
     (vendor/symfony/http-kernel/HttpKernel.php:74)
  at Symfony\Component\HttpKernel\HttpKernel->handle()
     (vendor/symfony/http-kernel/Kernel.php:184)
  at Symfony\Component\HttpKernel\Kernel->handle()
     (public/index.php:68)                

Here:

private function getSystemCollectionManager(): SystemCollectionManagerInterface
{
return $this->get('sulu_media.system_collections.manager');
}
/**
* Get media manager.
*/
private function getMediaManager(): MediaManagerInterface
{
return $this->get('sulu_media.media_manager');
}

instead of call $this->get() it should be $this->container->get().

@Jupi007
Copy link
Contributor

Jupi007 commented Feb 27, 2023

I just found another problem.

When a form isn't valid, an http 422 code need to be returned.
It is required by Turbo, otherwise invalid form errors isn't handled correctly.

This is automatically done by the controller if you use renderForm() + $form, instead of render() + $form->createView().
https://symfony.com/doc/5.4/forms.html#rendering-forms

@mario-fehr mario-fehr mentioned this pull request Mar 9, 2023
1 task
@alexander-schranz
Copy link
Member

Sorry for the late response here.

First really thank you for the pull request. We are still Supporting Symfony 5.4 and SwiftMailer in this repository. Also the old symfony security System (PasswordEncoder). So the following things would be required to be tackled:

For the 422 status code I would create a seperate issue and PR as that should be out of scope of this update. /cc @Jupi007

@Jupi007
Copy link
Contributor

Jupi007 commented Mar 30, 2023

For the 422 status code I would create a seperate issue and PR as that should be out of scope of this update. /cc @Jupi007

@alexander-schranz Okay, makes sense.
But as this feature is only available starting from Sf5.4, it will required to wait for this PR to be merged before fix it.

Edit: I created the issue #165

@alexander-schranz
Copy link
Member

@Jupi007 i would not even use that method just make sure that the 422 is returned in case of errors.

@Jupi007
Copy link
Contributor

Jupi007 commented Mar 30, 2023

Oh, you're right. I hadn't thought at this possibility.
In that case, this could be done at once.

@MemoICT
Copy link

MemoICT commented Apr 12, 2023

Any news here ?

@wachterjohannes
Copy link
Member

@alexander-schranz i have fixed a small issue with the completion listener! the rest of this branch works well. please review the code and merge it when its ok for you

@alexander-schranz
Copy link
Member

alexander-schranz commented Nov 20, 2023

@Zahiraa
Copy link

Zahiraa commented Dec 21, 2023

is there a an available version compatible with Sulu 2.5 and symfony 6 ?

@frickelbruder
Copy link

frickelbruder commented Mar 12, 2024

Hi all,

I played around with the PR and stumbled upon some problems, if you really still want to maintain SwiftMailer compatibility.
The requested BC-Layer code might be the smallest thing, as it is just copy and paste and surround with an if statement and replace the type hint with a union type. I could do that easily.

But when running the tests with swiftmailler installed, I got the following behaviour:

  1. If we use swiftmailer/swiftmailer as dependency, the sulu/sulu-TestBundle wants to include swiftmailer.yaml, which of course cannot handle bundle configs.
  2. So integrating symfony/swiftmailer-bundle helps. BUT this whole process downgrades the symfony kernel, which adds another odd situation: The Kernel from 6.4.5 to 5.4.37, which leads to a bug in the bc-layer in sulu core in File src/Sulu/Bundle/TestBundle/Resources/app/config/security-5-4.yml where the encoders node was renamed to password_hashers from Symfony 5.3. (By the way, the same bug is introduced in this PR)

So I am currently unsure, how you want to proceed here @alexander-schranz . To me it seems, this whole compatiblity layer does not work anyway. And I am unsure, as of how much work, I should put into this, with hopefully sulu 3 looming on the horizon anyway.

As I am in need of this bundle, I would be willing to put some effort in this.

@@ -198,7 +198,7 @@ private function addPermissions(RoleInterface $role, string $system, string $web
}
}

if (0 === \strpos($securityContext, 'sulu.webspaces.')
if (\str_starts_with($securityContext, 'sulu.webspaces.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not possible with PHP 7.2 which is still supported /cc @martinlagler

@martinlagler martinlagler force-pushed the 3.0 branch 5 times, most recently from 639fdd1 to 40e6096 Compare April 10, 2024 11:35
@martinlagler martinlagler force-pushed the 3.0 branch 7 times, most recently from 2061319 to c269d91 Compare April 10, 2024 13:56
@alexander-schranz alexander-schranz changed the title Compatibility with Sulu 2.5 - Symfony 6 Add compatibility with Sulu 2.5 - Symfony 6 Apr 10, 2024
@alexander-schranz alexander-schranz merged commit 723b4b4 into sulu:2.0 Apr 25, 2024
6 checks passed
@alexander-schranz
Copy link
Member

Thx @dev-newvisibility @martinlagler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants